feat: semi-additive measures - #2502
betodealmeida wants to merge 19 commits into
Conversation
✅ Deploy Preview for thriving-cassata-78ae72 canceled.
|
# Conflicts: # datajunction-server/datajunction_server/models/deployment.py
872a8e9 to
ca4d879
Compare
ca4d879 to
5b3ba3f
Compare
733896b to
17e0ca9
Compare
| ctx.dimensions.append(dimension) | ||
|
|
||
| # A second load_nodes pass is needed when either: | ||
| # 1. metric expressions introduced dimension nodes not yet in ctx.nodes, OR |
There was a problem hiding this comment.
If this is just a parent-column protected dimension (e.g., it's not the fully qualified node name like v3.order_details.order_date but just order_date), then it validates cleanly here but fails at query time:
POST /nodes/metric/
{"query": "SELECT SUM(line_total) FROM v3.order_details",
"reaggregate": {"rules": [{"dimension": "order_date", "fn": "last_value"}]}}
-> 201, status: valid
GET /sql/metrics/v3/?metrics=v3.balance&dimensions=v3.product.category
-> 422 "Reference `order_date` is not fully qualified. Use the `node.column` form..."Should this just reject a non-fully-qualified name (and I think the UI might need to change based on that as well)?
| if (values.upstream_node) { | ||
| const data = await djClient.node(values.upstream_node); | ||
| setDimensionOptions( | ||
| data.columns.map(col => ({ |
There was a problem hiding this comment.
This is related to the above comment -- here it's not producing fully qualified column names, just the column name itself, but then those metrics will fail to generate sql.
|
|
||
| # base_metrics exposes metric columns, not raw component/grain columns. | ||
| # Reaggregate leaf metrics from those projected metric values. | ||
| return ast.Function(ast.Name("SUM"), args=[metric_ref]) |
There was a problem hiding this comment.
Hmm, does this make distinct counts additive? For example let's say base_metrics is grouped by (date_id, category, week) and the outer query
collapses date_id away. If we have this setup:
| date | visitor_count |
|---|---|
| 2026-01-05 | 2 |
| 2026-01-06 | 1 |
The change here will return SUM(visitor_count) which isn't the distinct visitor count. The removed code here had a special case for Aggregability.LIMITED with COUNT(DISTINCT grain_col).
To be fair I think the old form was broken too, since base_metrics projects visitor_count and not customer_id, so COUNT(DISTINCT base_metrics.customer_id) referenced a missing column and would have errored.
Since this is in a path unrelated to semi-additive measures... could it be split out? We can have a separate PR that addresses this issue with its own tests.
| # Extract just the column names from dimensions for grain analysis | ||
| dim_column_names = [parse_dimension_ref(d).column_name for d in ctx.dimensions] | ||
| grain_groups = analyze_grain_groups(metric_group, dim_column_names) | ||
| output_dimensions = list(ctx.dimensions) |
There was a problem hiding this comment.
Hmm, it looks like a filter on the protected dimension will make the metric additive?
For example if we test with the same metric (v3.daily_balance) + dimension but with and without a filter:
correct metrics=[v3.daily_balance] + dimensions=[v3.product.category]
WITH
v3_order_details AS (
SELECT o.order_date,
oi.product_id,
oi.quantity * oi.unit_price AS line_total
FROM default.v3.orders o JOIN default.v3.order_items oi ON o.order_id = oi.order_id
),
v3_product AS (
SELECT product_id,
category
FROM default.v3.products
),
order_details_0 AS (
SELECT t2.category,
t1.order_date date_id_order,
SUM(t1.line_total) line_total_sum_e1f61696
FROM v3_order_details t1 LEFT OUTER JOIN v3_product t2 ON t1.product_id = t2.product_id
GROUP BY t2.category, t1.order_date
)
SELECT order_details_0.category AS category,
MAX_BY(order_details_0.line_total_sum_e1f61696, order_details_0.date_id_order) AS daily_balance
FROM order_details_0
GROUP BY order_details_0.categoryincorrect metrics=[v3.daily_balance] + dimensions=[v3.product.category] + filters=["v3.date.date_id[order] >= 20260101"]
-- order_date disappears from the CTE's GROUP BY and MAX_BY becomes SUM
WITH
v3_order_details AS (
SELECT oi.product_id,
oi.quantity * oi.unit_price AS line_total
FROM default.v3.orders o JOIN default.v3.order_items oi ON o.order_id = oi.order_id
WHERE o.order_date >= 20260101
),
v3_product AS (
SELECT product_id,
category
FROM default.v3.products
),
order_details_0 AS (
SELECT t2.category,
SUM(t1.line_total) line_total_sum_e1f61696
FROM v3_order_details t1 LEFT OUTER JOIN v3_product t2 ON t1.product_id = t2.product_id
GROUP BY t2.category
)
SELECT order_details_0.category AS category,
SUM(order_details_0.line_total_sum_e1f61696) AS daily_balance
FROM order_details_0
GROUP BY order_details_0.categorySo "daily balance by category" gives the latest balance per
category, and "daily balance by category, for January" gives the sum of every
daily balance in January, which is a different metric.
I think it does work for the cube path though, because build_synthetic_grain_group strips filter dims first.
| _raise_if_frozen_measure_conflicts(frozen_measure, measure) | ||
| if not frozen_measure and measure.aggregation: | ||
| frozen_measure = FrozenMeasure( | ||
| name=measure.name, |
There was a problem hiding this comment.
The two frozen-measure paths (deployment vs direct metrics creation) seem to differ:
- direct metrics creation (this one) keeps the reaggregate:
rule=measure.rule - deployment strips it with
rule=_frozen_measure_rule(measure.rule)
I think stripping makes more sense since the reaggregate isn't a property of the measure but rather something that can be applied on top
| message=f"Cube node `{name}` does not exist.", | ||
| http_status_code=404, | ||
| ) | ||
| await _validate_cube_reaggregate_materialization(session, node) |
There was a problem hiding this comment.
Should this part first call the pre-check cube_matcher._metric_graph_has_reaggregate (since it's already used in other APIs like in find_matching_cube)?
| Declaration for how a metric rolls up from its accumulation grain. | ||
| """ | ||
|
|
||
| fn: ReaggregationFunction | None = None |
There was a problem hiding this comment.
Arefn and weight used right now in sql gen or is this meant to be for later? Wonder if it's worth dropping until there's a specific use for these fields
| Return the registered source column alias for a dimension ref. | ||
| """ | ||
| alias = ctx.alias_registry.get_alias(dimension_ref) | ||
| if alias or _dimension_ref_role(dimension_ref) is not None: |
There was a problem hiding this comment.
it looks like when the protected dimension has a role and there's no alias in the registry, we'll end up falling through to SUM(metric_ref). Should this raise an "unsupported semi-additive shape" error in that case?
| ReaggregateRequirement = tuple[str, str, ReaggregationFunction] | ||
|
|
||
|
|
||
| def _split_dimension_ref(ref: str) -> tuple[str, str | None]: |
There was a problem hiding this comment.
Instead of adding another dimension-with-role parser, can this just reuse parse_dimension_ref from construction/build_v3/dimensions.py instead?
It actually looks like there's quite a few versions of this function that got added here:
_dimension_ref_base/_dimension_ref_role(inbuild_v3/decomposition.py)_dimension_ref_roleagain in (build_v3/metrics.py)_split_dimension_refhere- an inline
dim_ref.rsplit("[", 1)inbuild_v3/dimensions.py
Summary
This PR adds metric reaggregation support for semi-additive measures, aligned with the proposal #2245. Metrics can declare protected dimensions, and the aggregation behavior changes when the query grain drops the dimension.
For example, suppose we have this metric:
Meaning:
If we get a request that includes the protected dimension:
We generate this SQL:
Because
date_id_orderis still in the output grain, there is no semi-additive collapse. Normal aggregation is safe.On the other hand, if we don't request the protected dimension we shouldn't aggregate over it:
The generated SQL:
Here, the user asked for
category only, but DJ keepsdate_id_orderas a private inner grain. Then it collapses each category’s daily balances withMAX_BY(value, date), meaning "take the value from the latest date."Without this, DJ would generate something like:
That would incorrectly sum balances across dates, which is wrong for snapshots or balances, for example.
I've updated the UI to show and allow setting the semi-additive dimension:
When editing:
Valid options:
Test Plan
make checkpassesmake testshows 100% unit test coverageDeployment Plan